Skip to content

Add client steps implementation to support existing functionalities#6966

Open
alfonso-noriega wants to merge 10 commits intomainfrom
03-10-add_client_steps_implementation_to_support_existing_functionalities
Open

Add client steps implementation to support existing functionalities#6966
alfonso-noriega wants to merge 10 commits intomainfrom
03-10-add_client_steps_implementation_to_support_existing_functionalities

Conversation

@alfonso-noriega
Copy link
Contributor

@alfonso-noriega alfonso-noriega commented Mar 10, 2026

Add build step infrastructure for extension asset management

WHY are these changes introduced?

This introduces a flexible build step system to handle various asset copying and processing needs for different extension types, replacing hardcoded build logic with configurable steps.

WHAT is this pull request doing?

  • Adds 'admin' to the CONFIG_EXTENSION_IDS array for admin extensions
  • Creates new build step executors:
    • executeIncludeAssetsStep - handles pattern-based, static, and config-key-driven asset copying with comprehensive test coverage
    • executeCopyStaticAssetsStep - copies static assets defined in extension build manifests
    • executeCreateTaxStubStep - generates minimal JavaScript stub files for tax calculation extensions
  • Implements a centralized step router (executeStepByType) that dispatches to appropriate handlers based on step type
  • Supports multiple inclusion strategies:
    • Pattern-based file selection using glob patterns
    • Static file/directory copying with optional destination paths
    • Configuration key resolution for dynamic path discovery
    • Configurable structure preservation and flattening options

How to test your changes?

  1. Create an extension with various asset types (static files, pattern-matched files, config-driven paths)
  2. Configure build steps using the new include_assets step type with different inclusion strategies
  3. Run the build process and verify assets are copied correctly to output directories
  4. Test with admin extensions to ensure they're recognized as config extensions
  5. Run the comprehensive test suite for include-assets-step.test.ts

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

alfonso-noriega commented Mar 10, 2026

@alfonso-noriega alfonso-noriega marked this pull request as ready for review March 10, 2026 08:35
@alfonso-noriega alfonso-noriega requested a review from a team as a code owner March 10, 2026 08:35
@github-actions
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2026

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 82.16% 15078/18352
🟡 Branches 74.6% 7456/9995
🟢 Functions 81.25% 3800/4677
🟢 Lines 82.55% 14253/17265

Test suite run success

3981 tests passing in 1526 suites.

Report generated by 🧪jest coverage report action from a6262e1

@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 9c2e1fd to 7e48497 Compare March 10, 2026 09:04
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from 16768f3 to 59f0139 Compare March 10, 2026 09:59
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 7e48497 to 159bb73 Compare March 10, 2026 09:59
@alfonso-noriega alfonso-noriega changed the base branch from 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations to graphite-base/6966 March 10, 2026 10:29
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 159bb73 to e6f3577 Compare March 10, 2026 10:29
@alfonso-noriega alfonso-noriega changed the base branch from graphite-base/6966 to 03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications March 10, 2026 10:29
@binks-code-reviewer
Copy link

binks-code-reviewer bot commented Mar 10, 2026

🤖 Code Review · #projects-dev-ai for questions
React with 👍/👎 or reply — all feedback helps improve the agent.

Complete - 2 findings

📋 History

❌ Failed → ✅ 1 findings → ❌ Failed → ❌ Failed → ✅ 1 findings → ✅ 2 findings

@alfonso-noriega alfonso-noriega force-pushed the 03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications branch from bbc0817 to 7b68eb7 Compare March 10, 2026 11:05
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from e6f3577 to 3f28156 Compare March 10, 2026 11:05
@alfonso-noriega alfonso-noriega changed the base branch from 03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications to graphite-base/6966 March 10, 2026 11:11
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 3f28156 to 44034c5 Compare March 10, 2026 11:11
@alfonso-noriega alfonso-noriega changed the base branch from graphite-base/6966 to 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations March 10, 2026 11:12
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from f9f9e63 to 59dcaa7 Compare March 10, 2026 11:24
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 44034c5 to 094a68c Compare March 10, 2026 11:24
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from 59dcaa7 to fb4c01d Compare March 10, 2026 11:35
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch 2 times, most recently from 0478573 to a8b39c0 Compare March 10, 2026 12:15
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from a872825 to 9688bf3 Compare March 18, 2026 17:50
@alfonso-noriega alfonso-noriega force-pushed the 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations branch from bc8f04a to 35aaabb Compare March 18, 2026 17:50
@alfonso-noriega alfonso-noriega changed the base branch from 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations to graphite-base/6966 March 19, 2026 10:19
@alfonso-noriega alfonso-noriega force-pushed the 03-10-add_client_steps_implementation_to_support_existing_functionalities branch from 9688bf3 to d42767e Compare March 19, 2026 10:19
@alfonso-noriega alfonso-noriega changed the base branch from graphite-base/6966 to 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations March 19, 2026 10:19
@alfonso-noriega alfonso-noriega changed the base branch from 03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations to graphite-base/6966 March 19, 2026 11:24
Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Review assisted by pair-review

alfonso-noriega and others added 3 commits March 24, 2026 18:13
…pattern copy

- Add sanitizeDestination() that strips '..' segments from destination
  fields and emits a warning when any are removed
- Sanitize entry.destination for all three inclusion types (pattern,
  static, configKey) before it reaches any path join
- Add copy-time bounds check in copyByPattern: skip any file whose
  resolved destPath escapes outputDir and warn

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace positional args with config objects in copyByPattern,
  copySourceEntry, and copyConfigKeyEntry to prevent argument-order
  mistakes (e.g. mixing include/ignore patterns)
- Make the static dispatch branch explicit with if (entry.type === 'static')
- Fix copySourceEntry: when destination is provided and source is a
  directory, use copyDirectoryContents + glob count instead of
  copyFile + hardcoded 1

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
alfonso-noriega and others added 6 commits March 24, 2026 19:15
…type

- Change StaticEntrySchema preserveStructure default from false to true
  so directory sources preserve their name in the output by default
  rather than silently merging contents into the output root
- Unwrap copyByPattern return from {filesCopied: number} to number,
  consistent with copySourceEntry and copyConfigKeyEntry

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract sanitizeDestination into sanitizeRelativePath in
  @shopify/cli-kit/node/path so it can be shared across the codebase
- Simplify copySourceEntry by resolving destPath and logMsg up front
  then dispatching once on file vs directory, eliminating the duplicated
  copyDirectoryContents + glob + return count pattern

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move each copy strategy into its own file under include-assets/:
- copy-by-pattern.ts  — glob-based file selection
- copy-source-entry.ts — explicit source path copy
- copy-config-key-entry.ts — config key resolution + nested path helpers

include-assets-step.ts is now a thin orchestrator holding only schemas
and executeIncludeAssetsStep. No behavior change.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
counts is (number | undefined)[] due to TypeScript not seeing the three
if-checks as exhaustive. Explicit generic <number> on reduce fixes it.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Direct tests for copyByPattern, copySourceEntry, and copyConfigKeyEntry
covering edge cases not exercised through the orchestrator tests:
collision deduplication, path traversal guard, filepath===destPath no-op,
all destination resolution branches, file vs directory sources,
destination param, nested [] flatten paths.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/path.d.ts
 import type { URL } from 'url';
 /**
  * Joins a list of paths together.
  *
  * @param paths - Paths to join.
  * @returns Joined path.
  */
 export declare function joinPath(...paths: string[]): string;
 /**
  * Normalizes a path.
  *
  * @param path - Path to normalize.
  * @returns Normalized path.
  */
 export declare function normalizePath(path: string): string;
 /**
  * Resolves a list of paths together.
  *
  * @param paths - Paths to resolve.
  * @returns Resolved path.
  */
 export declare function resolvePath(...paths: string[]): string;
 /**
  * Returns the relative path from one path to another.
  *
  * @param from - Path to resolve from.
  * @param to - Path to resolve to.
  * @returns Relative path.
  */
 export declare function relativePath(from: string, to: string): string;
 /**
  * Returns whether the path is absolute.
  *
  * @param path - Path to check.
  * @returns Whether the path is absolute.
  */
 export declare function isAbsolutePath(path: string): boolean;
 /**
  * Returns the directory name of a path.
  *
  * @param path - Path to get the directory name of.
  * @returns Directory name.
  */
 export declare function dirname(path: string): string;
 /**
  * Returns the base name of a path.
  *
  * @param path - Path to get the base name of.
  * @param ext - Optional extension to remove from the result.
  * @returns Base name.
  */
 export declare function basename(path: string, ext?: string): string;
 /**
  * Returns the extension of the path.
  *
  * @param path - Path to get the extension of.
  * @returns Extension.
  */
 export declare function extname(path: string): string;
 /**
  * Parses a path into its components (root, dir, base, ext, name).
  *
  * @param path - Path to parse.
  * @returns Parsed path object.
  */
 export declare function parsePath(path: string): {
     root: string;
     dir: string;
     base: string;
     ext: string;
     name: string;
 };
 /**
  * Given an absolute filesystem path, it makes it relative to
  * the current working directory. This is useful when logging paths
  * to allow the users to click on the file and let the OS open it
  * in the editor of choice.
  *
  * @param path - Path to relativize.
  * @param dir - Current working directory.
  * @returns Relativized path.
  */
 export declare function relativizePath(path: string, dir?: string): string;
 /**
  * Given 2 paths, it returns whether the second path is a subpath of the first path.
  *
  * @param mainPath - The main path.
  * @param subpath - The subpath.
  * @returns Whether the subpath is a subpath of the main path.
  */
 export declare function isSubpath(mainPath: string, subpath: string): boolean;
 /**
  * Given a module's import.meta.url it returns the directory containing the module.
  *
  * @param moduleURL - The value of import.meta.url in the context of the caller module.
  * @returns The path to the directory containing the caller module.
  */
 export declare function moduleDirectory(moduleURL: string | URL): string;
 /**
  * When running a script using `npm run`, something interesting happens. If the current
  * folder does not have a `package.json` or a `node_modules` folder, npm will traverse
  * the directory tree upwards until it finds one. Then it will run the script and set
  * `process.cwd()` to that folder, while the actual path is stored in the INIT_CWD
  * environment variable (see here: https://docs.npmjs.com/cli/v9/commands/npm-run-script#description).
  *
  * @returns The path to the current working directory.
  */
 export declare function cwd(): string;
 /**
  * Tries to get the value of the `--path` argument, if provided.
  *
  * @param argv - The arguments to search for the `--path` argument.
  * @returns The value of the `--path` argument, if provided.
  */
 export declare function sniffForPath(argv?: string[]): string | undefined;
 /**
  * Returns whether the `--json` or `-j` flags are present in the arguments.
  *
  * @param argv - The arguments to search for the `--json` and `-j` flags.
  * @returns Whether the `--json` or `-j` flag is present in the arguments.
  */
 export declare function sniffForJson(argv?: string[]): boolean;
+/**
+ * Removes any `..` traversal segments from a relative path and calls `warn`
+ * if any were stripped. Normal `..` that cancel out within the path (e.g.
+ * `foo/../bar` → `bar`) are collapsed but never allowed to escape the root.
+ * Both `/` and `\` are treated as separators for cross-platform safety.
+ *
+ * @param input - The relative path to sanitize.
+ * @param warn - Called with a human-readable warning when traversal segments are removed.
+ * @returns The sanitized path (may be an empty string if all segments were traversal).
+ */
+export declare function sanitizeRelativePath(input: string, warn: (msg: string) => void): string;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants